-
-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework some PawnColumnWorker syncing #502
base: master
Are you sure you want to change the base?
Rework some PawnColumnWorker syncing #502
Conversation
This change is primarily to fix `PawnColumnWorker_Designator` and `PawnColumnWorker_Sterilize` displaying confirmation dialogs, which caused issues as pressing the checkbox would open the dialog for all players and the dialogs themselves weren't synced. I've mentioned this issue in rwmt#429. The first change is to stop syncing `PawnColumnWorker_Designator.SetValue` and `PawnColumnWorker_Sterilize.SetValue`, as those could display confirmation dialogs. Instead of those 2 methods, we instead sync: - `DesignationManager.AddDesignation` - This one is not needed, but was included for consistency (and may come in handy for mod compat, it can be safely removed if desired) - `DesignationManager.RemoveDesignation` - Called from `PawnColumnWorker_Designator.SetValue` when value is false - `PawnColumnWorker_Designator.DesignationConfirmed` - This method calls `DesignationManager.AddDesignation` (along with another method), which is why that specific method is not needed - `PawnColumnWorker_Sterilize.AddSterilizeOperation` - `PawnColumnWorker_Sterilize.CancelSterilizeOperations` This required adding extra sync worker delegates for `Designation` and `DesignationManager`. By not syncing the `SetValue` method, it allows for a potential multiple calls to the other synced methods which generally don't have checks if the state already matches. This requires additional patches that cancel execution if it would cause issues (`PreventPawnTableDesignationErrors`, `PreventPawnTableMultipleSterilizeOperations`). Finally, by not syncing the `SetValue` methods we don't call `SetDirty` on the pawn tables. To fix this I've added a method (`TryDirtyCurrentPawnTable`) which is called in post invoke for the synced methods, as well as after syncing designators, to cause the tables to re-sort their values. This will cause the tables to be re-sorted in a few extra situations (like when a different player modifies designators outside of pawn tables). It may be expanded to include more methods to cause the tables to be re-sorted when they normally wouldn't be in vanilla (if we so desire). Alternatively, this could be reduced or removed if we don't want it.
I haven't considered how map syncing works when writing those 2 sync worker delegates, and made an error in the way they handle maps. Specifically, the issue here is that writing a null map may result in a non-null map when reading if another sync worker is syncing the map. Likewise, I believe attempting to sync a null map may have set the map to null for a different sync worker delegate, causing issues there. The change here is to, rather than sync the map itself (which we may have potentially synced as null and caused issues for other sync workers) we instead sync a bool that determines if the manager is null or has null map (and we then set the MpContext to use its map).
I've fixed syncing null Designation/DesignationManager. This should fix issues with Vanilla Vehicles Expanded - Tier 3. I originally haven't considered how map syncing works when writing those 2 sync worker delegates, and made an error in the way they handle maps. Specifically, the issue here is that writing a null map may result in a non-null map when reading if another sync worker is syncing the map. Likewise, I believe attempting to sync a null map may have set the map to null for a different sync worker delegate, causing issues there. The change here is to, rather than sync the map itself (which we may have potentially synced as null and caused issues for other sync workers) we instead sync a bool that determines if the manager is null or has null map (and we then set the MpContext to use its map). |
This change is primarily to fix
PawnColumnWorker_Designator
andPawnColumnWorker_Sterilize
displaying confirmation dialogs, which caused issues as pressing the checkbox would open the dialog for all players and the dialogs themselves weren't synced. I've mentioned this issue in #429.The first change is to stop syncing
PawnColumnWorker_Designator.SetValue
andPawnColumnWorker_Sterilize.SetValue
, as those could display confirmation dialogs. Instead of those 2 methods, we instead sync:DesignationManager.AddDesignation
DesignationManager.RemoveDesignation
(and may come in handy for mod compat, it can be safely removed if desired)DesignationManager.RemoveDesignation
PawnColumnWorker_Designator.SetValue
when value is falsePawnColumnWorker_Designator.DesignationConfirmed
DesignationManager.AddDesignation
(along with another method), which is why that specific method is not needed for syncingPawnColumnWorker_Sterilize.AddSterilizeOperation
PawnColumnWorker_Sterilize.CancelSterilizeOperations
This required adding extra sync worker delegates for
Designation
andDesignationManager
. Those were taken from MP Compat (rwmt/Multiplayer-Compatibility#394).By not syncing the
SetValue
method, it allows for a potential multiple calls to the other synced methods which generally don't have checks if the state already matches. This requires additional patches that cancel execution if it would cause issues (PreventPawnTableDesignationErrors
,PreventPawnTableMultipleSterilizeOperations
).Finally, by not syncing the
SetValue
methods we don't callSetDirty
on the pawn tables. To fix this I've added a method (TryDirtyCurrentPawnTable
) which is called in post invoke for the synced methods, as well as after syncing designators, to cause the tables to re-sort their values. This will cause the tables to be re-sorted in a few extra situations (like when a different player modifies designators outside of pawn tables). It may be expanded to include more methods to cause the tables to be re-sorted when they normally wouldn't be in vanilla (if we so desire). Alternatively, this could be reduced or removed if we don't want it.